Isolate browser WebKit process pools#4987
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBrowserPanel now creates and uses a per-instance WKProcessPool, adds state and a public API for manual recovery after WKWebView web-content termination, surfaces UI to trigger recovery, closes popups on their web-content termination, and adds tests plus project wiring for these behaviors. ChangesPer-panel process isolation and termination recovery
Sequence Diagram(s)sequenceDiagram
participant User
participant BrowserPanel
participant WKWebView
User->>BrowserPanel: normal navigation / actions
BrowserPanel->>WKWebView: create WebView(processPool: panel.processPool)
WKWebView->>BrowserPanel: webContentProcessDidTerminate
BrowserPanel->>BrowserPanel: compute/store pendingWebContentRecoveryURL, set hasRecoverableWebContentTermination
User->>BrowserPanel: recoverTerminatedWebContent(reason)
BrowserPanel->>WKWebView: recreate WebView (using panel.processPool) and navigate to recovery URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR isolates each browser panel into its own
Confidence Score: 4/5Safe to merge with one open localization gap already flagged in a prior review comment: the new recovery overlay's tooltip uses browser.reload, which is still missing its Khmer translation. The process-pool isolation, manual recovery flow, popup cleanup, and profile/workspace reset paths are all correctly implemented and covered by the new test suite. The only outstanding gap is the browser.reload tooltip missing a km entry for the new recovery overlay — this was flagged in a previous review cycle and has not been addressed in this commit. Resources/Localizable.xcstrings — browser.reload is missing the km entry that was added to browser.error.reload in this PR. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WebContent Process Terminates] --> B[replaceWebViewAfterContentProcessTermination]
B --> C[replaceWebViewPreservingState\nwaitForManualRecovery: true]
C --> D{wasRenderable &&\nhasRecoveryTarget?}
D -- No --> E[clearWebContentTerminationRecovery\nrefreshNavigationAvailability]
D -- Yes --> F[pendingWebContentRecoveryURL = restoreURL\nhasRecoverableWebContentTermination = true]
F --> G[Show Reload Overlay]
G --> H{User action}
H -- Overlay / Toolbar / panel.reload --> I[recoverTerminatedWebContent]
H -- Navigate to new URL --> J[navigateWithoutInsecureHTTPPrompt\nclearWebContentTerminationRecovery]
H -- Profile Switch --> K[clearWebContentTerminationRecovery]
H -- Workspace Reset --> L[resetForWorkspaceContextChange\nclearWebContentTerminationRecovery]
I --> M[clearWebContentTerminationRecovery\nDismiss overlay]
M --> N{recoveryURL?}
N -- Yes --> O[navigateWithoutInsecureHTTPPrompt to recoveryURL]
N -- No --> P[refreshNavigationAvailability]
subgraph Process Isolation
Q[BrowserPanel init] --> R[new WKProcessPool per panel]
R --> S[makeWebView with per-panel pool]
S --> T[Popup inherits opener pool via popupBrowserContext]
end
Reviews (14): Last reviewed commit: "Clear web content recovery on profile sw..." | Re-trigger Greptile |
lawrencecchen
force-pushed
the
task-webview-crash-isolation
branch
from
fddf122 to
99dfbd4
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 4921-4922: The manual-recovery gate currently uses only
waitForManualRecovery && wasRenderable, which can mark an about:blank/URL-less
view as recoverable; update the shouldShowManualRecovery condition to also
verify there is a real navigation target from sessionNavigationHistorySnapshot()
(e.g., check history has a current entry/url or hasEntries/canNavigate flag)
before presenting recovery UI, and apply the same check in the other occurrence
(around lines referenced 4976-4984) so recoverTerminatedWebContent() only runs
when an actual recoverable URL exists.
- Around line 4914-4918: When building restoreURL in BrowserPanel.swift, prefer
the attempted URL if a provisional main-frame navigation is active: check
navigationDelegate?.isMainFrameProvisionalNavigationActive and, when true, place
navigationDelegate?.lastAttemptedURL (and its remoteProxyDisplayURL) before
currentURL in the fallback chain used to compute restoreURL; otherwise keep the
existing ordering. Update the restoreURL expression that calls
Self.remoteProxyDisplayURL(for:), currentURL,
navigationDelegate?.lastAttemptedURL, and resolvedCurrentSessionHistoryURL() to
branch on isMainFrameProvisionalNavigationActive and swap the attempted-URL
entries into the first fallback positions.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 1661-1681: The recovery overlay's Label uses the localized key
"browser.error.reload" (seen in webContentRecoveryOverlay) but the Khmer locale
is missing in Resources/Localizable.xcstrings; add a "km" entry for
"browser.error.reload" with the correct Khmer translation (matching the format
used for other locales), ensure the string encoding/quoting matches existing
.xcstrings entries and include the plural/variant if applicable so the Label
shows Khmer instead of falling back to English.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94a99af9-8c0f-45cf-9315-27954b49ca52
📒 Files selected for processing (6)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/BrowserPopupWindowController.swiftcmuxTests/BrowserConfigTests.swiftcmuxTests/BrowserPanelTests.swiftcmuxTests/GhosttyConfigTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/BrowserWebContentProcessTests.swift`:
- Around line 29-32: Tests use a real network URL ("https://example.com") when
creating BrowserPanel instances which can introduce flakiness; update the
BrowserPanel initialURL arguments in the referenced spots (the BrowserPanel
initializers at the occurrences around the BrowserPanel creation and the other
two occurrences) to use a deterministic in-process URL such as "about:blank" or
a local file:// fixture instead of an external HTTPS URL so the recovery tests
don't depend on outbound networking or navigation timing.
- Around line 99-111: Add a positive precondition that the popup is actually
presented before simulating process termination: after creating popupWebView via
panel.createFloatingPopup and obtaining popupWindow, assert that
popupWindow.isVisible (or another positive signal such as popupWebView.window !=
nil or popupWebView.superview != nil) is true, then call
popupWebView.navigationDelegate?.webViewWebContentProcessDidTerminate?(popupWebView)
and finally assert the popup closed; update the test around createFloatingPopup,
popupWebView, popupWindow and the termination invocation to include this initial
visibility check so the final `#expect`(!popupWindow.isVisible) cannot pass
spuriously.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 2951-2953: The shared static fallbackConfigurationProcessPool is
overwriting a copied WKWebViewConfiguration's existing processPool when callers
omit processPool; change logic so we only assign
fallbackConfigurationProcessPool to configurations that have a nil processPool
and only when creating configurations for utility callers (e.g., in BrowserPanel
methods that use fallbackConfigurationProcessPool). Locate uses of
fallbackConfigurationProcessPool and the code path that mutates
WKWebViewConfiguration.processPool; update it to check configuration.processPool
== nil before assigning, preserving any pre-existing processPool on copied
configurations (also apply same check in the other occurrences around the lines
mentioned).
- Around line 3109-3110: The UI binding is stale because
hasRecoverableWebContentTermination is not observable; mark it as `@Published`
(e.g. change "private(set) var hasRecoverableWebContentTermination = false" to
"`@Published` private(set) var hasRecoverableWebContentTermination = false") so
BrowserPanelView receives updates; if the view also reads
pendingWebContentRecoveryURL consider making that property `@Published` as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1fbbb35-b320-408f-ac53-ad62f740995a
📒 Files selected for processing (3)
Sources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserWebContentProcessTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Panels/BrowserPanel.swift (2)
5807-5825:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear recovery state before queueing a deferred remote navigation.
performNavigation()clearshasRecoverableWebContentTermination, but theusesRemoteWorkspaceProxy && remoteProxyEndpoint == nilpath returns before it runs. After a crash on a remote workspace, typing a new URL while the proxy is disconnected leaves the stale recovery overlay armed until reconnect instead of treating the new navigation as the replacement action.Suggested fix
private func navigateWithoutInsecureHTTPPrompt( request: URLRequest, recordTypedNavigation: Bool, preserveRestoredSessionHistory: Bool = false ) { guard let url = request.url else { return } cancelHiddenWebViewDiscard() clearWebViewDiscardState(reason: "navigation") + clearWebContentTerminationRecovery() if usesRemoteWorkspaceProxy, remoteProxyEndpoint == nil { pendingRemoteNavigation = PendingRemoteNavigation( request: request, recordTypedNavigation: recordTypedNavigation, preserveRestoredSessionHistory: preserveRestoredSessionHistory🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/BrowserPanel.swift` around lines 5807 - 5825, navigateWithoutInsecureHTTPPrompt returns early when usesRemoteWorkspaceProxy && remoteProxyEndpoint == nil, but it doesn't clear the recoverable-crash state; replicate what performNavigation does by clearing hasRecoverableWebContentTermination (or invoking the same helper used there) before queuing pendingRemoteNavigation so typing a new URL disarms the stale recovery overlay; update the block in navigateWithoutInsecureHTTPPrompt (around pendingRemoteNavigation and hiddenWebViewDiscardManager.updateRestoredSessionRenderIntent) to clear that recovery flag.
5193-5200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPublish the chosen recovery target before arming manual recovery.
When a provisional main-frame load crashes,
restoreURLcan point at the attempted page whilecurrentURLstill points at the last committed page. In this branch you saverestoreURLintopendingWebContentRecoveryURLbut leavecurrentURLunchanged, so the omnibar and any session snapshot taken before the user clicks Reload still describe the wrong page.Suggested fix
if shouldShowManualRecovery, let restoreURL { + currentURL = restoreURL pendingWebContentRecoveryURL = restoreURL hasRecoverableWebContentTermination = true refreshNavigationAvailability() } else {Also applies to: 5255-5258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/BrowserPanel.swift` around lines 5193 - 5200, The chosen recovery target (restoreURL) must be published before arming manual recovery so the omnibar and session snapshot reflect the recovery page; move the assignment that publishes the recovery target (set pendingWebContentRecoveryURL or call the method that publishes it) to occur immediately after computing restoreURL/restoreURLString and before setting waitForManualRecovery/arming manual recovery (the branch that computes shouldShowManualRecovery/shouldRestoreURL), and do the same change for the repeated block around the 5255-5258 region so the UI/current session is updated (e.g., currentURL/omnibar state or session snapshot publish) before you enable manual recovery.
♻️ Duplicate comments (1)
cmuxTests/BrowserWebContentProcessTests.swift (1)
128-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncomplete: Check visibility, not just window attachment.
Line 128 verifies the window is attached (
popupWebView.window === popupWindow), but doesn't prove the popup was actually visible before termination. The previous review comment specifically requested a visibility check to ensure the test can't pass spuriously.Add
#expect(popupWindow.isVisible)here to confirm the popup is open before simulating termination at Line 130.🔍 Proposed fix to add visibility precondition
let popupWindow = try `#require`(popupWebView.window) `#expect`(popupWebView.window === popupWindow) +#expect(popupWindow.isVisible) popupWebView.navigationDelegate?.webViewWebContentProcessDidTerminate?(popupWebView)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/BrowserWebContentProcessTests.swift` at line 128, The test only asserts the popup view is attached to the window (popupWebView.window === popupWindow) but not that the popup was visible; update the test to assert visibility by adding a precondition check using popupWindow.isVisible (e.g., add `#expect`(popupWindow.isVisible) immediately after the existing popupWebView.window assertion) so the popup is confirmed visible before the termination simulation, keeping the existing attachment assertion intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 5807-5825: navigateWithoutInsecureHTTPPrompt returns early when
usesRemoteWorkspaceProxy && remoteProxyEndpoint == nil, but it doesn't clear the
recoverable-crash state; replicate what performNavigation does by clearing
hasRecoverableWebContentTermination (or invoking the same helper used there)
before queuing pendingRemoteNavigation so typing a new URL disarms the stale
recovery overlay; update the block in navigateWithoutInsecureHTTPPrompt (around
pendingRemoteNavigation and
hiddenWebViewDiscardManager.updateRestoredSessionRenderIntent) to clear that
recovery flag.
- Around line 5193-5200: The chosen recovery target (restoreURL) must be
published before arming manual recovery so the omnibar and session snapshot
reflect the recovery page; move the assignment that publishes the recovery
target (set pendingWebContentRecoveryURL or call the method that publishes it)
to occur immediately after computing restoreURL/restoreURLString and before
setting waitForManualRecovery/arming manual recovery (the branch that computes
shouldShowManualRecovery/shouldRestoreURL), and do the same change for the
repeated block around the 5255-5258 region so the UI/current session is updated
(e.g., currentURL/omnibar state or session snapshot publish) before you enable
manual recovery.
---
Duplicate comments:
In `@cmuxTests/BrowserWebContentProcessTests.swift`:
- Line 128: The test only asserts the popup view is attached to the window
(popupWebView.window === popupWindow) but not that the popup was visible; update
the test to assert visibility by adding a precondition check using
popupWindow.isVisible (e.g., add `#expect`(popupWindow.isVisible) immediately
after the existing popupWebView.window assertion) so the popup is confirmed
visible before the termination simulation, keeping the existing attachment
assertion intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d2e17a9-d344-44be-bf28-5c73662ca743
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserWebContentProcessTests.swift
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 5261-5269: When replacing the WKWebView during the restore/swap
path, ensure you clear the browser focus flags by calling
clearBrowserFocusMode(reason: ...) as part of the swap flow; locate the
swap/replace branch that runs navigateWithoutInsecureHTTPPrompt(...) (the path
handling restoreURL or the else refreshNavigationAvailability branch) and insert
a call to clearBrowserFocusMode(reason: <appropriate existing enum case used for
discard/context reset, e.g. .webContentProcessTerminated or .discarded>) before
rebinding the new WKWebView so isBrowserFocusModeActive /
isBrowserFocusModeExitArmed are reset immediately after the old view is
destroyed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37c1f017-e28c-454e-8ad9-568f1c557397
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserWebContentProcessTests.swift
lawrencecchen
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
Dismissed stale CodeRabbit changes-requested review after all inline comments were addressed or resolved and the latest PR checks passed.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7802ef3. Configure here.
lawrencecchen
force-pushed
the
task-webview-crash-isolation
branch
from
f3a1d89 to
652a87a
Compare
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WKProcessPoolso one bad page is less likely to take sibling browser panels down with itTesting
./scripts/reload.sh --tag webcrash./scripts/reload.sh --tag webcrash --launchhttp://127.0.0.1:18787/?cap=4096; it pushed the WebContent process to roughly 2.1 GiB RSS while cmux stayed around 430 MiB RSS.SIGKILLto the probe WebContent PID; cmux stayed alive, switching to a terminal tab worked, andecho cmux survived WebContent SIGKILLran successfully.cmux-aws-m4prohad only 363 MiB free on/after cleanup.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches core WebKit lifecycle, navigation restore, and popup teardown; behavior change from shared pools may affect cookie/process assumptions across tabs while data stores stay shared.
Overview
Each browser panel now gets its own
WKProcessPoolinstead of sharing one app-wide pool, so a WebContent crash is meant to stay confined to that tab while profileWKWebsiteDataStoresharing is unchanged. Popups still use the opener’s pool viaBrowserPopupBrowserContextand are closed when their WebContent process terminates.After a content-process death, the panel rebuilds
WKWebViewon the same pool but does not auto-reload; it sets recoverable state, picks a smarter restore URL (including in-flight navigations), and shows a Reload overlay. Toolbar/menu reload andrecoverTerminatedWebContentperform the actual navigation; profile switch, workspace reset, and normal navigation clear pending recovery.configureWebViewConfigurationnow takes an optional process pool (omit to preserve an existing pool). NewBrowserWebContentProcessTestsand a Khmer Reload string were added.Reviewed by Cursor Bugbot for commit 652a87a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Isolated each browser panel into its own
WKProcessPoolso crashes only affect that tab, and added a manual recovery flow with a Reload overlay; toolbar/menu Reload attempts recovery first. Popups stay in the opener’s context and close if their web content process terminates.New Features
WKProcessPoolwith shared-profileWKWebsiteDataStore.WKWebViewon the same pool, clears loading/progress, and shows a Reload overlay; smarter restore URL (includes in‑flight navigations).recoverTerminatedWebContent; recovery is cleared on navigation, profile switch, and workspace reset.Refactors
configureWebViewConfigurationtakes optionalprocessPool, preserving an existing pool and removing a redundant overload; avoids default-argument warnings.WKWebViewreplacement to prevent stuck focus.Written for commit 652a87a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Localization
Tests